core: add devtools path to DOMStats #11578

adrianaixba · 2020-10-19T17:25:06Z

Adds devtools node path to DOMStats gatherer. Allows for devtools linkification on report.

adrianaixba · 2020-10-19T17:29:49Z

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

-      pathToElement: elementPathInDOM(deepestElement),
-      // ignore style since it will provide no additional context, and is often long
-      snippet: getOuterHTMLSnippet(deepestElement, ['style']),
+      devtoolsNodePath: getNodePath(deepestElement),


I was hoping to use getNodeDetails to get all the info. But, interestingly while running a smoke test (dbw), there was a case when the nodeSelector function failed because the node/element had an undefined tagName? So, I decided to fall back to just gathering the devtoolsNodePath & path.

It looks like that is because the element being resolved is a ShadowRoot not an Element. ShadowRoot does not have a tagName field so getNodeDetails runs into an error in the functions that use tagName. In the smoke test, the element with most children happens to be the host of a shadow dom.

I think getNodeDetails can work if you pass in the shadow root host when parentWithMostChildren is a ShadowRoot not an Element. You could also change getNodeSelector and getNodeLabel to handle a ShadowRoot being passed in. getOuterHTMLSnippet and getNodePath already do this which explains why those functions were working on their own:

lighthouse/lighthouse-core/lib/page-functions.js

Lines 115 to 125 in 8095bec

function getOuterHTMLSnippet(element, ignoreAttrs = [], snippetCharacterLimit = 500) {

const ATTRIBUTE_CHAR_LIMIT = 75;

// Autofill information that is injected into the snippet via AutofillShowTypePredictions

// TODO(paulirish): Don't clean title attribute from all elements if it's unnecessary

const autoFillIgnoreAttrs = ['autofill-information', 'autofill-prediction', 'title'];

try {

// ShadowRoots are sometimes passed in; use their hosts' outerHTML.

if (element instanceof ShadowRoot) {

element = element.host;

}

Thanks for the explanation this worked! I'm wondering, though, why the shadowRoot case might have not been considered for the other functions (including getBoundingClientRect(element)), if it were on purpose or just a case that hadn't been encountered before - which I'm thinking might be the case.

adamraine · 2020-10-19T19:36:59Z

lighthouse-core/audits/dobetterweb/dom-size.js

+          type: 'node',
+          path: stats.depth.devtoolsNodePath,
+          snippet: stats.depth.snippet,
+        }),
        statistic: str_(UIStrings.statisticDOMDepth),
        element: {


You are replacing element with node correct? You shouldn't need element in the items list anymore.

adamraine · 2020-10-19T22:19:39Z

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

-      pathToElement: elementPathInDOM(deepestElement),
-      // ignore style since it will provide no additional context, and is often long
-      snippet: getOuterHTMLSnippet(deepestElement, ['style']),
+      devtoolsNodePath: getNodePath(deepestElement),


It looks like that is because the element being resolved is a ShadowRoot not an Element. ShadowRoot does not have a tagName field so getNodeDetails runs into an error in the functions that use tagName. In the smoke test, the element with most children happens to be the host of a shadow dom.

I think getNodeDetails can work if you pass in the shadow root host when parentWithMostChildren is a ShadowRoot not an Element. You could also change getNodeSelector and getNodeLabel to handle a ShadowRoot being passed in. getOuterHTMLSnippet and getNodePath already do this which explains why those functions were working on their own:

lighthouse/lighthouse-core/lib/page-functions.js

Lines 115 to 125 in 8095bec

function getOuterHTMLSnippet(element, ignoreAttrs = [], snippetCharacterLimit = 500) {

const ATTRIBUTE_CHAR_LIMIT = 75;

// Autofill information that is injected into the snippet via AutofillShowTypePredictions

// TODO(paulirish): Don't clean title attribute from all elements if it's unnecessary

const autoFillIgnoreAttrs = ['autofill-information', 'autofill-prediction', 'title'];

try {

// ShadowRoots are sometimes passed in; use their hosts' outerHTML.

if (element instanceof ShadowRoot) {

element = element.host;

}

…ighthouse into domstats-devtools

… expectations

adamraine · 2020-10-22T20:23:36Z

lighthouse-core/lib/page-functions.js

@@ -295,6 +295,7 @@ function getNodeSelector(node) {
   * @param {Element} node
   */
  function getSelectorPart(node) {
+    node = node instanceof ShadowRoot ? node.host : node;


If a ShadowRoot instance is passed in to getNodeSelector, it will output the selector part of the shadow host and then exit.

Sorry, I take back what I said about changing these functions to accomodate ShadowRoot. I it makes more senes to pass node.host into getNodeDetails when node is an instance of ShadowRoot. This way we can remove these lines and don't have to change jsdoc to be Element|ShadowRoot everywhere.

WDYT about getNodeDetails doing this check? Historically when the responsibility to support ShadowRoots is pushed to the caller of a function it's almost always forgotten :)

SGTM. Sorry for changing so frequently @adrianaixba 😅

@adamraine no worries! The more discussion, the better 😄

lighthouse-core/audits/dobetterweb/dom-size.js

adamraine · 2020-10-23T19:31:55Z

Looks like the reason getOuterHTMLSnippet accepts a ShadowRoot is because of dom stats sometimes passing one in: #9079

@brendankenny @patrickhulce Does it make sense to move the if(element instanceof ShadowRoot) logic from getOuterHTMLSnippet to the dom stats gatherer?

patrickhulce · 2020-10-23T19:59:43Z

Does it make sense to move the if(element instanceof ShadowRoot) logic from getOuterHTMLSnippet to the dom stats gatherer?

That's one option. I think the broader problem though is exactly what @adrianaixba stumbled on to. It's not super clear what our existing gatherers assume/support about ShadowRoots and prior to #9079 roots were still getting passed into that fn without any type errors (I assume due to our sad "@ts-ignore put into scope via stringification" which means these arguments are never really type checked 😢 ).

I would have a preference for normalizing Element|ShadowRoot into Element in any publicly exposed methods in page-functions to avoid the caller having to remember that responsibility in every gatherer.

adrianaixba · 2020-10-24T00:05:25Z

lighthouse-core/lib/page-functions.js

-    selector: getNodeSelector(elem),
-    boundingRect: getBoundingClientRect(elem),
+    selector: getNodeSelector(shadowRootElem),
+    boundingRect: getBoundingClientRect(shadowRootElem),
    snippet: getOuterHTMLSnippet(elem),


decided to leave these as is, since they take care of the shadowRoot case. @adamraine @patrickhulce what do y'all think?

getNodePath will return different results for ShadowRoot and its host element, so leaving that as is was a good call.

@patrickhulce Would you prefer keeping the ShadowRoot logic in getOuterHTMLSnippet even though it won't be used anywhere after this patch?

I agree, I wouldn't go out of our way to remove ShadowRoot support from anything just yet, even if the path is currently being covered by our other logic. If we also removed the functions from the public interface of this module, then I'd feel less strongly, but seems like that step would benefit from a more targeted and comprehensive plan for what to do with shadow elements across all of lighthouse that feels out of scope for this PR.

adamraine

Whoops, lost track of this 😬

Overall LGTM

adamraine · 2020-11-06T20:54:24Z

lighthouse-core/test/results/artifacts/defaultPass.trace.json

-
-  {"_comment": "Manually added event to make sample lhr not error", "name":"largestContentfulPaint::Candidate", "pid":75994,"tid":17667,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline", "args": {"frame": "0x44d2861df8"}},
-  {"_comment": "Manually added event to make test CLS", "name":"LayoutShift", "pid":75994,"tid":775,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline", "args": {"frame": "0x44d2861df8", "data": {"is_main_frame": true, "cumulative_score": 0.42}}},
-


This seems unrelated. I don't mind the style change, but is it related to this patch somehow?

@adamraine This is unrelated. It's just a style change that has continued to pop up for me for some reason, I could revert.

I think the change is good, its probably too small for its own PR so I'm ok leaving it in this one :)

adamraine · 2020-11-06T21:04:08Z

lighthouse-core/lib/page-functions.js

@@ -453,12 +452,13 @@ const getNodeDetailsString = `function getNodeDetails(elem) {
  ${getBoundingClientRect.toString()};
  ${getOuterHTMLSnippet.toString()};
  ${getNodeLabel.toString()};
+  const shadowRootElem = elem instanceof ShadowRoot ? elem.host : elem;


nit: the value of shadowRootElem will not necessarily be a shadow root. I think something like htmlElem makes more sense.

connorjclark · 2020-11-06T21:24:45Z

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

-
-  return name;
-}
-


adamraine

LGTM

adrianaixba added 2 commits October 19, 2020 12:10

domstats gatherer: using node path instead of custom

938ec86

small update

8850d72

adrianaixba requested a review from a team as a code owner October 19, 2020 17:25

adrianaixba requested review from adamraine and removed request for a team October 19, 2020 17:25

google-cla bot added the cla: yes label Oct 19, 2020

devtools-bot assigned adamraine Oct 19, 2020

devtools-bot added the waiting4reviewer label Oct 19, 2020

adrianaixba commented Oct 19, 2020

View reviewed changes

small update

23ccc4b

vercel bot deployed to Preview October 19, 2020 17:34 View deployment

adrianaixba changed the title ~~Domstats devtools~~ core: add devtools path to DOMStats Oct 19, 2020

small update

e8c87ee

vercel bot deployed to Preview October 19, 2020 17:42 View deployment

adamraine reviewed Oct 19, 2020

View reviewed changes

adamraine added waiting4committer and removed waiting4reviewer labels Oct 20, 2020

adrianaixba added 2 commits October 22, 2020 10:05

removed table element remnants

de5bd61

Merge branch 'domstats-devtools' of https://github.com/GoogleChrome/l…

7071ceb

…ighthouse into domstats-devtools

vercel bot deployed to Preview October 22, 2020 15:09 View deployment

reimplemented getNodeDetails for shadowRoot case, adjusted smoke test…

e1e9d98

… expectations

vercel bot deployed to Preview October 22, 2020 17:21 View deployment

cleanup artifacts & domstats

ce2fdde

vercel bot deployed to Preview October 22, 2020 17:59 View deployment

cleanup

002fc49

vercel bot deployed to Preview October 22, 2020 18:39 View deployment

adrianaixba added waiting4reviewer and removed waiting4committer labels Oct 22, 2020

adamraine reviewed Oct 22, 2020

View reviewed changes

adrianaixba removed the waiting4reviewer label Oct 22, 2020

adrianaixba added the waiting4committer label Oct 22, 2020

moved shadowroot logic

16221a9

vercel bot deployed to Preview October 23, 2020 23:41 View deployment

cleanup

a7b8fa4

vercel bot deployed to Preview October 24, 2020 00:00 View deployment

adrianaixba commented Oct 24, 2020

View reviewed changes

adrianaixba added waiting4reviewer and removed waiting4committer labels Oct 26, 2020

adamraine reviewed Nov 6, 2020

View reviewed changes

adamraine added waiting4committer and removed waiting4reviewer labels Nov 6, 2020

connorjclark reviewed Nov 6, 2020

View reviewed changes

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

return name;

}

Copy link

Collaborator

connorjclark Nov 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love it

adrianaixba reacted with laugh emoji

cleanups

cf7a4bf

vercel bot deployed to Preview November 9, 2020 21:21 View deployment

adamraine approved these changes Nov 9, 2020

View reviewed changes

adrianaixba merged commit a73e19a into master Nov 9, 2020

adrianaixba deleted the domstats-devtools branch November 9, 2020 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add devtools path to DOMStats #11578

core: add devtools path to DOMStats #11578

adrianaixba commented Oct 19, 2020

adrianaixba Oct 19, 2020

adamraine Oct 19, 2020 •

edited

Loading

adrianaixba Oct 22, 2020

adamraine Oct 19, 2020

adamraine Oct 19, 2020 •

edited

Loading

adamraine Oct 22, 2020

patrickhulce Oct 23, 2020

adamraine Oct 23, 2020

adrianaixba Oct 23, 2020 •

edited

Loading

adamraine commented Oct 23, 2020 •

edited

Loading

patrickhulce commented Oct 23, 2020

adrianaixba Oct 24, 2020

adamraine Oct 26, 2020 •

edited

Loading

patrickhulce Oct 26, 2020

adamraine left a comment

adamraine Nov 6, 2020

adrianaixba Nov 9, 2020

adamraine Nov 9, 2020

adamraine Nov 6, 2020

connorjclark Nov 6, 2020

adamraine left a comment

	function getOuterHTMLSnippet(element, ignoreAttrs = [], snippetCharacterLimit = 500) {
	const ATTRIBUTE_CHAR_LIMIT = 75;
	// Autofill information that is injected into the snippet via AutofillShowTypePredictions
	// TODO(paulirish): Don't clean title attribute from all elements if it's unnecessary
	const autoFillIgnoreAttrs = ['autofill-information', 'autofill-prediction', 'title'];

	try {
	// ShadowRoots are sometimes passed in; use their hosts' outerHTML.
	if (element instanceof ShadowRoot) {
	element = element.host;
	}


		{"_comment": "Manually added event to make sample lhr not error", "name":"largestContentfulPaint::Candidate", "pid":75994,"tid":17667,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline", "args": {"frame": "0x44d2861df8"}},
		{"_comment": "Manually added event to make test CLS", "name":"LayoutShift", "pid":75994,"tid":775,"ts":185608247190,"ph":"R","cat":"loading,rail,devtools.timeline", "args": {"frame": "0x44d2861df8", "data": {"is_main_frame": true, "cumulative_score": 0.42}}},

core: add devtools path to DOMStats #11578

core: add devtools path to DOMStats #11578

Conversation

adrianaixba commented Oct 19, 2020

Choose a reason for hiding this comment

adamraine Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianaixba Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

adamraine commented Oct 23, 2020 • edited Loading

patrickhulce commented Oct 23, 2020

Choose a reason for hiding this comment

adamraine Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine left a comment

Choose a reason for hiding this comment

adamraine Oct 19, 2020 •

edited

Loading

adamraine Oct 19, 2020 •

edited

Loading

adrianaixba Oct 23, 2020 •

edited

Loading

adamraine commented Oct 23, 2020 •

edited

Loading

adamraine Oct 26, 2020 •

edited

Loading